Android: Enable views to be nested within <Text> #8619

rigdern · 2016-07-06T21:39:21Z

Potential breaking change: The signature of ReactShadowNode's onBeforeLayout method was changed

Before: public void onBeforeLayout()
After: public void onBeforeLayout(NativeViewHierarchyOptimizer nativeViewHierarchyOptimizer)

Depends on this css-layout PR:
facebook/yoga#202

Implements same feature as this iOS PR:
#7304

Previously, only Text and Image could be nested within Text. Now, any view can be nested within Text. One restriction of this feature is that developers must give inline views a width and a height via the style prop. Here's a screenshot illustrating this feature:

Note that the changes to NativeViewHierarchyOptimizer were broken up across a few commits to make the diffs easier to understand.

Previously, inline Images were supported via FrescoBasedReactTextInlineImageSpan. To get support for nesting views within Text, we create one special kind of span per inline view. This span is called TextInlineViewPlaceholderSpan. It is the same size as the inline view. Its job is just to occupy space -- it doesn't render any visual. After the text is rendered, we query the Android Layout object associated with the TextView to find out where it has positioned each TextInlineViewPlaceholderSpan. We then position the views to be at those locations.

One tricky aspect of the implementation is that the Text component needs to be able to render native children (the inline views) but the Android TextView cannot have children. This is solved by having the native parent of the ReactTextView also host the inline views. Implementation-wise, this was accomplished by extending the NativeViewHierarchyOptimizer to handle this case. The optimizer now handles these cases:

Node is not in the native tree. An ancestor must host its children.
Node is in the native tree and it can host its own children.
(new) Node is in the native tree but it cannot host its own children. An ancestor must host both this node and its children.

Limitation: Clipping

If Text's height/width is small such that an inline view doesn't completely fit, the inline view may still be fully visible due to hoisting (the inline view isn't actually parented to the Text which has the limited size. It is parented to an ancestor which may have a different clipping rectangle.). Prior to this change, layout-only views had a similar limitation.

Test plan (required)

This change was tested with UIExplorer and a small test app and it's being used in my team's app.

Outstanding Issue: Using Text and Image within an inline view

Currently, you cannot use Text or Image components within an inline view. This is because the React context flag, isInAParentText, indicates that you're still inside of a Text component and these components use their virtual node representation rather than their normal representation.

The Text component sets isInAParentText to true. No components ever set isInAParentText to false.

I need some help with fixing this. One option I was considering was setting isInAParentText to false for all other native components. This could be done by editing the native component creation primitives such as requireNativeComponent and createReactNativeComponentClass to provide the appropriate context. However, when I tried this, it didn’t work and it wasn’t clear to me whether or not context works on native components. Thoughts?

Adam Comella
Microsoft Corp.

ghost · 2016-07-06T21:40:34Z

By analyzing the blame information on this pull request, we identified @dmmiller and @korDen to be potential reviewers.

dmmiller · 2016-07-07T12:13:00Z

This is a lot, I'll look tomorrow (Friday)

dmmiller · 2016-07-08T14:47:31Z

I'm looking through this. It might actually be better to have this as separate PRs. When we import them, all the commits get squashed into one request. Would probably be easier to get through.

dmmiller · 2016-07-08T14:48:43Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java

@@ -348,6 +355,15 @@ public ReactTextShadowNode(boolean isVirtual) {
    }
  }

+  // Returns a line height which takes into account the requested line height
+  // and the height of the inline images.
+  public float getAffectiveLineHeight() {


should be effectiveLineHeight

dmmiller · 2016-07-08T14:59:27Z

The first commit seems good (modulo the one thing inline)

dmmiller · 2016-07-08T15:11:18Z

The second commit is solid. Nice refactoring.

dmmiller · 2016-07-08T15:12:51Z

Ditto for third.

rigdern · 2016-07-12T21:39:32Z

@dmmiller I haven't seen any updates in a few days. Are you still reviewing this or are you waiting for me to take some action?

dmmiller · 2016-07-19T10:18:24Z

@rigdern Sorry, was on holiday last week. I thought I noted that here, but I didn't. I have looked at the first three commits. I'd actually prefer them to go in separately because we actually squash any PR into one commit so we would end up with a giant one. They seem independent and good cleanup and I can quickly stamp them since I've already looked at them. Then you can rebase the fourth and just that one can be looked at in more detail. Again sorry for the delay, I thought I had mentioned I was on holiday but forgot.

rigdern · 2016-07-20T00:47:29Z

@dmmiller Okay, here's what I did. I addressed your feedback, rebased all of my commits onto the latest master, and tested the changes. Then I split this into 2 other PRs:

Android: Fix handling of line height with inline images #8907: Line height fix for inline images
Android: Refactor HierarchyOptimizer in preparation of inline view support #8908: Refactoring of HierarchyOptimizer

This PR still includes the other 3 commits (because it builds on them) but it has been rebased onto the latest master. You can review the last commit (Enable views to be nested within ). After the other PRs are accepted, I will rebase this on master and remove the other 3 commits from this PR.

In addition to reviewing the last commit, a couple of other things still need to happen:

Merge PR Java: Expose extra functionality on CSSNode yoga#202 into css-layout and then import it into react-native.
Solve the outstanding issue for using Text and Image within an inline view. See my initial PR description for details.

dmmiller · 2016-07-20T10:49:46Z

Accepted those two PRs

I don't know about css, @emilsjolander is the right person for that, and I assume you are working with him on the css PR.

emilsjolander · 2016-07-20T10:55:07Z

Yup, looking at merging css-layout end of this or early next week.

…pport Summary: This PR was split from commits originally in #8619. /cc dmmiller These refactorings to the HierarchyOptimizer are in preparation for implementing support for inline views in #8619. **Refactoring 1: Collapse add*LayoutOnlyNodeToLayoutOnlyNode** addLayoutOnlyNodeToLayoutOnlyNode and addNonLayoutOnlyNodeToLayoutOnlyNode had nearly identical implementations. They both walk thru the ancestors looking for a nonlayout-only node and adjusting the passed in index at each step. This introduces a new function, walkUpUntilNonLayoutOnly, which takes care of that responsibility. This simplifies addNodeToNode because it can now consider the type of the parent and the type of the child independently. **Refactoring 2: Extract addGrandchildren** Pull out addLayoutOnlyNode's logic into a helper called addGrandchildren. We will need to call this method in another place later. **Test plan (required)** This change was tested with UIExplorer and a small test app and it's being used in my team's app. Closes #8908 Differential Revision: D3592783 Pulled By: dmmiller fbshipit-source-id: a513e8d381e71112ce6348bbee7d4a7c62c33619

Summary: This PR was split from a commit originally in #8619. /cc dmmiller When an inline image was larger than the specified line height, the image would be clipped. This changes the behavior so that the line height is changed to make room for the inline image. This is consistent with the behavior of RN for iOS. Here's how the change works. ReactTextView now receives its line height from the layout thread rather than directly from JavaScript. The reason is that the layout thread may pick a different line height. In the case that the tallest inline image is larger than the line height supplied by JavaScript, we want to use that image's height as the line height rather than the supplied line height. Also fixed a bug where the image, which is supposed to be baseline aligned, would be positioned at the wrong y location. To fix this, we use `y` (the baseline) in the `draw` method rather than trying to calculate the baseline from `bottom`. For more information see https://code.google.com/p/andro Closes #8907 Differential Revision: D3592781 Pulled By: dmmiller fbshipit-source-id: cba6cd86eb4e3abef6a0d7a81f802bdb0958492e

rigdern · 2016-08-12T19:02:00Z

there's always the possibility of writing your own view manager for your own inline image/drawable subclass.

In order for this to work, we need to implement either option (2) custom Drawables or (3) inline views.

I think my litmus test would be whether we can add support for them without adding new code paths we have to maintain within the core layouting and UI code.

I can look into doing an implementation of inline views in which Text components which have inline views get wrapped by a View. That way, we won't have to make changes to the NativeViewHierarchyOptimizer and might be able to keep the changes contained to the Text component.

@astreet, can you find somebody who can help give advice on the following problems (they might look familiar because they were mentioned earlier in this issue)?:

Using Text and Image within an inline view. Currently, you cannot use Text or Image components within an inline view. This is because the React context flag, isInAParentText, indicates that you're still inside of a Text component and these components use their virtual node representation rather than their normal representation. The Text component sets isInAParentText to true. No components ever set isInAParentText to false. I need some help with fixing this. One option I was considering was setting isInAParentText to false for all other native components. This could be done by editing the native component creation primitives such as requireNativeComponent and createReactNativeComponentClass to provide the appropriate context. However, when I tried this, it didn’t work and it wasn’t clear to me whether or not context works on native components.
Inspecting native primitives in JavaScript. Can we have the JavaScript logic for the render function of Text components output different things depending on whether or not all of its children are strings/Texts/RawTexts? The problem I faced is that this.props.children contains a mixture of native primitives (e.g. View, Text, ScrollView) and components (subclasses of React.Component). When you encounter a React.Component, you want to know what native primitives it'll render down into. I don't know of any easy way of getting at this information in JavaScript.

ghost · 2016-08-20T17:01:42Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @emilsjolander as a potential reviewer. Could you take a look please or cc someone with more context?

astreet · 2016-08-23T20:52:13Z

Sorry about the delay on this, I was out:

In order for this to work, we need to implement either option (2) custom Drawables or (3) inline views.

When we're talking about adding specifically image-rendering functionality that we don't think is necessarily useful or worth 'paying for' to the core library, custom Drawables doesn't seem bad to me. Currently, you would be able to create a custom ViewManager (a la https://github.com/facebook/react-native/blob/1926fdf156e077ad8b0c73505c6447d1a16687f8/ReactAndroid/src/main/java/com/facebook/react/views/text/frescosupport/FrescoBasedReactTextInlineImageViewManager.java) and extend ReactTextInlineImageShadowNode (see https://github.com/facebook/react-native/search?utf8=%E2%9C%93&q=ReactTextInlineImageShadowNode&type=Code)

Given the difficulties you're listing (which I don't think are easily solvable), at this point I feel pretty confident that adding animated inline image support specifically is the right call here.

…pport Summary: This PR was split from commits originally in facebook#8619. /cc dmmiller These refactorings to the HierarchyOptimizer are in preparation for implementing support for inline views in facebook#8619. **Refactoring 1: Collapse add*LayoutOnlyNodeToLayoutOnlyNode** addLayoutOnlyNodeToLayoutOnlyNode and addNonLayoutOnlyNodeToLayoutOnlyNode had nearly identical implementations. They both walk thru the ancestors looking for a nonlayout-only node and adjusting the passed in index at each step. This introduces a new function, walkUpUntilNonLayoutOnly, which takes care of that responsibility. This simplifies addNodeToNode because it can now consider the type of the parent and the type of the child independently. **Refactoring 2: Extract addGrandchildren** Pull out addLayoutOnlyNode's logic into a helper called addGrandchildren. We will need to call this method in another place later. **Test plan (required)** This change was tested with UIExplorer and a small test app and it's being used in my team's app. Closes facebook#8908 Differential Revision: D3592783 Pulled By: dmmiller fbshipit-source-id: a513e8d381e71112ce6348bbee7d4a7c62c33619

Summary: This PR was split from a commit originally in facebook#8619. /cc dmmiller When an inline image was larger than the specified line height, the image would be clipped. This changes the behavior so that the line height is changed to make room for the inline image. This is consistent with the behavior of RN for iOS. Here's how the change works. ReactTextView now receives its line height from the layout thread rather than directly from JavaScript. The reason is that the layout thread may pick a different line height. In the case that the tallest inline image is larger than the line height supplied by JavaScript, we want to use that image's height as the line height rather than the supplied line height. Also fixed a bug where the image, which is supposed to be baseline aligned, would be positioned at the wrong y location. To fix this, we use `y` (the baseline) in the `draw` method rather than trying to calculate the baseline from `bottom`. For more information see https://code.google.com/p/andro Closes facebook#8907 Differential Revision: D3592781 Pulled By: dmmiller fbshipit-source-id: cba6cd86eb4e3abef6a0d7a81f802bdb0958492e

…pport Summary: This PR was split from commits originally in facebook#8619. /cc dmmiller These refactorings to the HierarchyOptimizer are in preparation for implementing support for inline views in facebook#8619. **Refactoring 1: Collapse add*LayoutOnlyNodeToLayoutOnlyNode** addLayoutOnlyNodeToLayoutOnlyNode and addNonLayoutOnlyNodeToLayoutOnlyNode had nearly identical implementations. They both walk thru the ancestors looking for a nonlayout-only node and adjusting the passed in index at each step. This introduces a new function, walkUpUntilNonLayoutOnly, which takes care of that responsibility. This simplifies addNodeToNode because it can now consider the type of the parent and the type of the child independently. **Refactoring 2: Extract addGrandchildren** Pull out addLayoutOnlyNode's logic into a helper called addGrandchildren. We will need to call this method in another place later. **Test plan (required)** This change was tested with UIExplorer and a small test app and it's being used in my team's app. Closes facebook#8908 Differential Revision: D3592783 Pulled By: dmmiller fbshipit-source-id: a513e8d381e71112ce6348bbee7d4a7c62c33619

Summary: This PR was split from a commit originally in facebook#8619. /cc dmmiller When an inline image was larger than the specified line height, the image would be clipped. This changes the behavior so that the line height is changed to make room for the inline image. This is consistent with the behavior of RN for iOS. Here's how the change works. ReactTextView now receives its line height from the layout thread rather than directly from JavaScript. The reason is that the layout thread may pick a different line height. In the case that the tallest inline image is larger than the line height supplied by JavaScript, we want to use that image's height as the line height rather than the supplied line height. Also fixed a bug where the image, which is supposed to be baseline aligned, would be positioned at the wrong y location. To fix this, we use `y` (the baseline) in the `draw` method rather than trying to calculate the baseline from `bottom`. For more information see https://code.google.com/p/andro Closes facebook#8907 Differential Revision: D3592781 Pulled By: dmmiller fbshipit-source-id: cba6cd86eb4e3abef6a0d7a81f802bdb0958492e

ghost · 2016-09-07T00:31:26Z

@rigdern updated the pull request - view changes

rigdern · 2016-09-07T00:40:44Z

I accidentally pushed to this branch. There's nothing new to review yet.

huangmr · 2016-10-06T15:38:05Z

any update on this PR please?

astreet · 2016-11-02T12:01:37Z

@huangmr: What are you trying to achieve? Would be good to know what other people are trying to do with this patch.

mkonicek · 2016-11-03T19:28:13Z

@rigdern I'm curious - did the iOS implementation end up getting landed again after being reverted? #7304

rigdern · 2016-11-03T19:46:45Z

@mkonicek Yes, 486dbe4 has been in RN since 0.28.0.

hramos · 2016-11-07T22:04:22Z

@rigdern do you still plan to add more commits to this PR or is it ready for another review pass?

rigdern · 2016-11-07T22:24:29Z

@hramos, last time @astreet reviewed this, he expressed concerns with the current approach and suggested a couple of other approaches. Unless opinions have changed on my original approach, I have to try out one of the other approaches and update the PR before it can be reviewed. Because investigating the other approaches will be time consuming and the current implementation is working well for my team's app, I haven't had the chance to revisit this PR yet.

lacker · 2016-12-14T23:23:26Z

It sounds like you might just never revisit this PR. Should we close it?

rigdern · 2016-12-14T23:32:39Z

@lacker I intend on coming back to this at some point. My team needs this feature and is currently using this PR in our app. We want this feature to be part of the official RN repo. Revisiting this PR will be time consuming so I can't justify spending time on it right now given the priorities of my other work.

If you'd like, I can close the PR and repoen it or open a new one when I revisit it. What do you think?

lacker · 2016-12-14T23:49:54Z

That sounds good. Let's open a new one when you revisit - I think that will just help us organizationally get a handle on our pull request backlog. Thanks for understanding!

Summary: This PR was split from a commit originally in #8619. /cc dmmiller When an inline image was larger than the specified line height, the image would be clipped. This changes the behavior so that the line height is changed to make room for the inline image. This is consistent with the behavior of RN for iOS. Here's how the change works. ReactTextView now receives its line height from the layout thread rather than directly from JavaScript. The reason is that the layout thread may pick a different line height. In the case that the tallest inline image is larger than the line height supplied by JavaScript, we want to use that image's height as the line height rather than the supplied line height. Also fixed a bug where the image, which is supposed to be baseline aligned, would be positioned at the wrong y location. To fix this, we use `y` (the baseline) in the `draw` method rather than trying to calculate the baseline from `bottom`. For more information see https://code.google.com/p/andro Closes #8907 Differential Revision: D3592781 Pulled By: dmmiller

Summary: This PR was split from a commit originally in facebook#8619. /cc dmmiller When an inline image was larger than the specified line height, the image would be clipped. This changes the behavior so that the line height is changed to make room for the inline image. This is consistent with the behavior of RN for iOS. Here's how the change works. ReactTextView now receives its line height from the layout thread rather than directly from JavaScript. The reason is that the layout thread may pick a different line height. In the case that the tallest inline image is larger than the line height supplied by JavaScript, we want to use that image's height as the line height rather than the supplied line height. Also fixed a bug where the image, which is supposed to be baseline aligned, would be positioned at the wrong y location. To fix this, we use `y` (the baseline) in the `draw` method rather than trying to calculate the baseline from `bottom`. For more information see https://code.google.com/p/andro Closes facebook#8907 Differential Revision: D3592781 Pulled By: dmmiller

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 6, 2016

rigdern force-pushed the rigdern/android-inline-views branch from 627c331 to 5b50a0e Compare July 6, 2016 21:44

dmmiller reviewed Jul 8, 2016
View reviewed changes

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 13, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 19, 2016

This was referenced Jul 19, 2016

Android: Fix handling of line height with inline images #8907

Closed

Android: Refactor HierarchyOptimizer in preparation of inline view support #8908

Closed

rigdern force-pushed the rigdern/android-inline-views branch from 5b50a0e to e96c15a Compare July 20, 2016 00:33

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 20, 2016

rigdern force-pushed the rigdern/android-inline-views branch from e96c15a to a49a4d9 Compare July 20, 2016 01:22

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 20, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 12, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 25, 2016

rigdern force-pushed the rigdern/android-inline-views branch 2 times, most recently from c74001d to 249ef50 Compare September 7, 2016 00:30

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2016

lacker added GH Review: needs-revision and removed GH Review: review-needed labels Oct 25, 2016

lacker closed this Dec 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Enable views to be nested within <Text> #8619

Android: Enable views to be nested within <Text> #8619

rigdern commented Jul 6, 2016

ghost commented Jul 6, 2016

dmmiller commented Jul 7, 2016

dmmiller commented Jul 8, 2016

dmmiller Jul 8, 2016

rigdern Jul 20, 2016

dmmiller commented Jul 8, 2016

dmmiller commented Jul 8, 2016

dmmiller commented Jul 8, 2016

rigdern commented Jul 12, 2016

dmmiller commented Jul 19, 2016

rigdern commented Jul 20, 2016

dmmiller commented Jul 20, 2016

emilsjolander commented Jul 20, 2016

rigdern commented Aug 12, 2016

ghost commented Aug 20, 2016

astreet commented Aug 23, 2016

ghost commented Sep 7, 2016

rigdern commented Sep 7, 2016

huangmr commented Oct 6, 2016

astreet commented Nov 2, 2016

mkonicek commented Nov 3, 2016

rigdern commented Nov 3, 2016

hramos commented Nov 7, 2016 •

edited

Loading

rigdern commented Nov 7, 2016

lacker commented Dec 14, 2016

rigdern commented Dec 14, 2016

lacker commented Dec 14, 2016

Android: Enable views to be nested within <Text> #8619

Android: Enable views to be nested within <Text> #8619

Conversation

rigdern commented Jul 6, 2016

ghost commented Jul 6, 2016

dmmiller commented Jul 7, 2016

dmmiller commented Jul 8, 2016

dmmiller Jul 8, 2016

Choose a reason for hiding this comment

rigdern Jul 20, 2016

Choose a reason for hiding this comment

dmmiller commented Jul 8, 2016

dmmiller commented Jul 8, 2016

dmmiller commented Jul 8, 2016

rigdern commented Jul 12, 2016

dmmiller commented Jul 19, 2016

rigdern commented Jul 20, 2016

dmmiller commented Jul 20, 2016

emilsjolander commented Jul 20, 2016

rigdern commented Aug 12, 2016

ghost commented Aug 20, 2016

astreet commented Aug 23, 2016

ghost commented Sep 7, 2016

rigdern commented Sep 7, 2016

huangmr commented Oct 6, 2016

astreet commented Nov 2, 2016

mkonicek commented Nov 3, 2016

rigdern commented Nov 3, 2016

hramos commented Nov 7, 2016 • edited Loading

rigdern commented Nov 7, 2016

lacker commented Dec 14, 2016

rigdern commented Dec 14, 2016

lacker commented Dec 14, 2016

hramos commented Nov 7, 2016 •

edited

Loading